Partial lightning network node implementation #1103

shamardy · 2021-10-12T07:11:12Z

This is the first of multiple PRs for lightning integration in mm2 #1045

Some notes:

There are multiple todos left in the code that should be done with the next phase which is implementing channels opening as they can be better tested after having channels.
I used dispatcher_legacy for the enable_lightning method for now as I am waiting to see how it will fit with enable_v2 in the future to move it to dispacher_v2
This is the guide I used for a big part of this code https://lightningdevkit.org/tutorials/build_a_node_in_rust/ it can help in understanding the code better
I implemented the node for electrum only for now as part of the PoC. In the future, I will implement the native mode too but I postponed it for now because I will need to run the bitcoin daemon in Regtest mode for it.

mm2src/coins/lightning.rs

artemii235

Thanks for the PR! I have a few questions and comments.

artemii235 · 2021-10-13T09:24:56Z

mm2src/coins/lightning.rs

+    // The current time is used to derive random numbers from the seed where required, to ensure all random generation is unique across restarts.
+    let cur = try_s!(SystemTime::now().duration_since(SystemTime::UNIX_EPOCH));
+
+    // Initialize the KeysManager


If we initialize BTC and LTC lightning at the same time, can this cause any problems or even be insecure?

I haven't thought about LTC lightning yet as it's not supported by the rust-lightning library. https://github.com/rust-bitcoin/rust-lightning/blob/fe8c10db95124e3238b7469bdabb00afc7c5bdd6/lightning-invoice/src/lib.rs#L328-L345.
But as I understand for LTC or any other coin (some other UTXO coins are currently experimenting with lightning e.g. Bitcoin Gold) we will have to run enable lightning again with a different port and configs so it will have a different KeysManager. I guess this https://github.com/KomodoPlatform/atomicDEX-API/blob/ad87f483cc5199851a781c5b99a1bda341f75ecb/mm2src/common/mm_ctx.rs#L98 will have to be changed to even run tBTC and BTC together in lightning

we will have to run enable lightning again with a different port and configs so it will have a different KeysManager

If these managers will run at the exactly same time, will they generate the same keys/secrets? If yes, are these secrets then revealed? Can this cause potential compromise of the LTC or another coin channel using information about the BTC channel?

It might be very hard to catch the same nanoseconds value, but do all the OSes we use support nanoseconds timestamp precision? According to https://doc.rust-lang.org/std/time/struct.Duration.html: If the underlying system does not support nanosecond-level precision, APIs binding a system timeout will typically round up the number of nanoseconds.

If these managers will run at the exactly same time, will they generate the same keys/secrets? If yes, are these secrets then revealed? Can this cause potential compromise of the LTC or another coin channel using information about the BTC channel?

I can't find an instance in the LDK code where these secrets are revealed so this should be alright. But for safety and to ensure uniqueness maybe we can keep the last generated value of SystemTime in the ctx and check when enabling 2 or more coins, this will require locking which might be even enough without storing the last value to ensure let cur = try_s!(SystemTime::now().duration_since(SystemTime::UNIX_EPOCH)); is unique as it will never run at the same time in different threads.

It might be very hard to catch the same nanoseconds value, but do all the OSes we use support nanoseconds timestamp precision? According to https://doc.rust-lang.org/std/time/struct.Duration.html: If the underlying system does not support nanosecond-level precision, APIs binding a system timeout will typically round up the number of nanoseconds.

Only Linux supports nanoseconds precision for the SystemTime::now() call according to the underlying system calls, for Windows and Darwin the precision is in microseconds. So for the above suggestion of locking the ctx when doing the SystemTime::now() it might be sufficient to wait a minimum of one microsecond before unlocking it.

I will add this to the TODO in the code when supporting multiple coins. I will keep this https://github.com/KomodoPlatform/atomicDEX-API/blob/ad87f483cc5199851a781c5b99a1bda341f75ecb/mm2src/common/mm_ctx.rs#L98 for now which means only one coin can be enabled.

artemii235 · 2021-10-13T09:27:42Z

mm2src/coins/lightning.rs

+    ));
+
+    // Initialize p2p networking
+    let listener = try_s!(TcpListener::bind(format!("0.0.0.0:{}", conf.ln_peer_listening_port)).await);


If our node is behind the NAT, can this affect Lightning functionality?

I am not sure as no real p2p functionality is working right now. It will appear in the next stage of the implementation as all lightning events require p2p messaging, and my computer is behind NAT so I can test it more extensively then. Maybe I should add a TODO note here to not forget.
I can also see that NAT Traversal is available for other LN implementations but can't find any mention of it in rust-lightning docs.
I found a mistake I made also, I think I need to send the external IP address here even if the node can't bind on it https://github.com/KomodoPlatform/atomicDEX-API/blob/ad87f483cc5199851a781c5b99a1bda341f75ecb/mm2src/coins/lightning.rs#L401-L405 I didn't do this as they said in the documentation that an empty IP Vec is alright https://github.com/rust-bitcoin/rust-lightning/blob/fe8c10db95124e3238b7469bdabb00afc7c5bdd6/lightning/src/ln/channelmanager.rs#L2370 but if the node is behind NAT other nodes still need the external IP address to connect to it, and the IP is also needed for the node to appear in the network graph as I can see from other parts of the rust-lightning library.

Fixed the mistake I mentioned above in the latest commits

mm2src/coins/lightning.rs

artemii235 · 2021-10-13T09:33:17Z

mm2src/coins/lightning/ln_rpc.rs

+}
+
+impl BroadcasterInterface for ElectrumClient {
+    fn broadcast_transaction(&self, tx: &Transaction) {


It's a bit sad that they assume a synchronous interface for this 🙁

I think this is intended as they leave you the choice to run these functions in asynchronous code through other interfaces. For the broadcast_transaction function for instance, when a new best block is updated for the channel manager https://github.com/KomodoPlatform/atomicDEX-API/blob/ad87f483cc5199851a781c5b99a1bda341f75ecb/mm2src/coins/lightning.rs#L384 if it's required to claim on-chain channel funds for some reason as this case https://github.com/rust-bitcoin/rust-lightning/blob/fe8c10db95124e3238b7469bdabb00afc7c5bdd6/lightning/src/chain/onchaintx.rs#L486, broadcast_transaction is run in the best_block_update thread away from the p2p networking and node announcement. The only problem I see is if for the same new best block more than one on-chain transaction needs to be broadcasted then they will be broadcasted synchronously inside the best_block_update thread but this is very rare but would have been solved if broadcast_transaction was async.

artemii235

Approving, thanks! The questions I still have are not blockers.

shamardy added 14 commits September 23, 2021 19:00

impl FeeEstimator, Logger, BroadcasterInterface

6991e16

impl Filter for ElectrumClient

3f72b8a

lightning conf + ChainMonitor for electrum

35473c8

fix wasm build

bbd4bdd

impl keys_manager and channel_manager

0db0f29

NetGraphMsgHandler, PeerManager, spawn network

7deb32e

update best block for chainmon and channel manager

36ff2ac

Mock LN events, Persist CM, Background Processing

9143078

1st version of enable_lightning rpc for test

c1482ef

refactoring + Error handling

94e7d74

remove hardcoded lightning conf paramaters

c64bd37

move LN trait impls for Electrum to seperate file

d2c2595

enable_lightning test

e61056a

Merge remote-tracking branch 'origin/dev' into ln-htlc-poc

ad87f48

shamardy requested review from artemii235, Milerius and sergeyboyko0791 October 12, 2021 07:25

Milerius reviewed Oct 12, 2021

View reviewed changes

mm2src/coins/lightning.rs Outdated Show resolved Hide resolved

mm2src/coins/lightning.rs Outdated Show resolved Hide resolved

artemii235 reviewed Oct 13, 2021

View reviewed changes

shamardy added 3 commits October 13, 2021 23:49

move enable_lightning to dispacher_v2

5ab74b0

announce external ip address to LN

d7b7c04

Add node color to configs and request

7c74dac

artemii235 approved these changes Oct 15, 2021

View reviewed changes

Milerius self-requested a review October 16, 2021 08:09

Milerius approved these changes Oct 16, 2021

View reviewed changes

artemii235 merged commit 637e247 into dev Oct 18, 2021

artemii235 deleted the ln-htlc-poc branch October 18, 2021 11:19

shamardy mentioned this pull request Nov 15, 2021

Lightning Network Channels #1133

Merged

shamardy mentioned this pull request Apr 20, 2022

[r2r] Lightning channels and payments history #1240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial lightning network node implementation #1103

Partial lightning network node implementation #1103

shamardy commented Oct 12, 2021

artemii235 left a comment

artemii235 Oct 13, 2021

shamardy Oct 13, 2021 •

edited

Loading

artemii235 Oct 15, 2021

shamardy Oct 15, 2021

shamardy Oct 15, 2021

artemii235 Oct 13, 2021

shamardy Oct 13, 2021 •

edited

Loading

shamardy Oct 14, 2021

artemii235 Oct 13, 2021

shamardy Oct 13, 2021

artemii235 left a comment

Partial lightning network node implementation #1103

Partial lightning network node implementation #1103

Conversation

shamardy commented Oct 12, 2021

artemii235 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemii235 left a comment

Choose a reason for hiding this comment

shamardy Oct 13, 2021 •

edited

Loading

shamardy Oct 13, 2021 •

edited

Loading